Ensure MatElem subtypes T have T(R::Ring, r::Int, c::Int) constructor
#1791
Conversation
| @@ -11,8 +11,7 @@ | |||
| ############################################################################### | |||
|
|
|||
| function similar(::AcbMatrix, R::AcbField, r::Int, c::Int) | |||
There was a problem hiding this comment.
In the future this and its many siblings could be replaced by a single default method
similar(::T, R::AcbField, r::Int, c::Int) where T <: MatElem = T(R, r, c)
| @@ -747,8 +746,7 @@ end | |||
| ############################################################################### | |||
|
|
|||
| function (x::AcbMatSpace)() | |||
There was a problem hiding this comment.
Currently we already have a default method for this which is
function (a::MatSpace{T})() where {T <: NCRingElement}
return zero_matrix(base_ring(a), nrows(a), ncols(a))::dense_matrix_type(T)
end
but that means we implicitly expect zero_matrix to be implemented... With the new system both can be implemented generically.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1791 +/- ##
==========================================
- Coverage 85.88% 85.86% -0.02%
==========================================
Files 95 95
Lines 36374 36388 +14
==========================================
+ Hits 31239 31246 +7
- Misses 5135 5142 +7 ☔ View full report in Codecov by Sentry. |
| # used by windows, not finalised!! | ||
| # MatElem interface | ||
| function FqMatrix(R::FqField, r::Int, c::Int) | ||
| return FqMatrix(r, c, R) |
There was a problem hiding this comment.
For FqMatrix we already have FqMatrix(r, c, R) but I don't see a downside with having both that and FqMatrix(R, r, c). Down the road we could think about abolishing the former in a breaking release, but there is no strong need either.
(One could also argue that we should just use the (r, c, R) order everywhere. Would be also OK to me. I used the former because (a) the old PR used it and (b) it matches the order in matrix(R, r, c, ...), zero_matrix, and some other places
I think @thofma added the FqMatrix constructor here, so perhaps he had a deeper reason for the different order that we should heed?
There was a problem hiding this comment.
The reason is that I copy & pasted it from some other matrix constructor in Nemo. There is no deeper reason. It is not part of the API, so can be changed/augmented/removed.
|
Before looking at this more closely, I wanted to ask what the overall aim of this is? If it is just about reducing the necessary functions in the matrix API, we could reduce everything to If we want to go down this route, maybe we can reuse the |
|
This goes back to this comment by @fieker which stated:
Actually, I understood Tommy's comment to be in the same vein, but re-reading it, I see now that it can also be interpreted as just saying "why not let I followed the idea here that I don't want to make anything worse than it is right now. Regarding adapting the Anyway, this kind of discussion is precisely why I did not want to complete this PR, just do enough to showcase the situation and we can hash it out. @fieker do you know from the top of your head if we have any existing examples of matrix types that actually allow creating an "uninitialized" matrix? Of course even if there are none, that's not a good reason to not plan for them. But it would allow setting up tests and to take some timings. |
In a nutshell, that's it, but the true goal then is to on the one hand make it easier to create further confirming matrix implementation, and on the other hand to remove various minor discrepancies between our existing implementations and to make it easier to implement "optimal" code in other situations (that's not yet in here of course). |
This would be a step towards resurrecting Nemocas/AbstractAlgebra.jl#558 i.e. to reduce the code
MatElemsubtypes must implement.In particular the new constructors would by default produce uninitialized matrices and then other constructors like
zero_matrix,identity_matrixorones_matrixcould be built atop. (In the first mentioned PR there is also a type traitis_zero_initialized(T::Type{<:MatrixElem})which can be used to "optimize" e.g.zero_matrixoridentity_matrixfor matrix types where the new matrices are zero matrices anyway).I am marking this as a draft because there are some call sites that could be adjusted, but before putting into any more effort into this, I wanted to check if this is a direction we want to go into to begin with.